Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removes item_subtype requirement for model-specific limits. #1309

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Removes item_subtype requirement for model-specific limits. #1309

merged 1 commit into from
Jul 26, 2021

Conversation

PhilCoggins
Copy link
Contributor

@PhilCoggins PhilCoggins commented Apr 14, 2021 •
edited
Loading

  • When enforcing limits for a model, it was previously required to
    have the item_subtype column on the versions table to obtain the
    configured limit for a given model type, especially for models
    subclassed via STI.

  • This change instead references the item on the version record,
    which is already available via the association cache, to determine
    the properly configured limit. This change drops the requirement of
    having item_subtype to configure model-specific limits.

Assuming there are no issues with simply referencing item.class to determine the configured limit, this should be a better setup, as it doesn't require item_subtype.

Note: The example that I removed assumes that in the absence of item_subtype, the global limit would be used - even if a model had its own configured limit. This spec was irrelevant because it was not possible to have simultaneously defined limit with has_paper_trail and not have item_subtype in your versions schema, because an exception would be thrown.

Fixes #1307

@PhilCoggins PhilCoggins force-pushed the remove_item_subtype_req_for_limit branch from ab83036 to d79d6d6 Compare April 14, 2021 00:49
@PhilCoggins
Copy link
Contributor Author

@jaredbeck Any thoughts on this approach?

@github-actions
Copy link

This PR has been automatically marked as stale due to inactivity.
The resources of our volunteers are limited.
If this is something you are committed to continue working on, please address any concerns raised by review and/or ping us again.
Thank you for all your contributions.

@github-actions github-actions bot added the Stale label Jul 15, 2021
@jaredbeck jaredbeck requested a review from aried3r July 15, 2021 02:13
@jaredbeck
Copy link
Member

@aried3r Do you have time to look at this? I've been busy with some other stuff lately.

@jaredbeck jaredbeck removed the Stale label Jul 15, 2021
@aried3r
Copy link
Member

aried3r commented Jul 21, 2021

I had a quick look. It seems reasonable and all specs pass. Are we perhaps lacking a spec that proves that it works without the item_subtype column now? Since the removed spec just documented that it was not possible before.

jaredbeck
jaredbeck requested changes Jul 25, 2021
View reviewed changes
lib/paper_trail/version_concern.rb Show resolved Hide resolved
lib/paper_trail/model_config.rb Show resolved Hide resolved
@jaredbeck
Copy link
Member

@PhilCoggins sorry for the delayed review. Please let us know if you have trouble making the requested changes.

@jaredbeck jaredbeck force-pushed the remove_item_subtype_req_for_limit branch from d79d6d6 to bca9aa5 Compare July 25, 2021 03:41
@jaredbeck
Copy link
Member

jaredbeck force-pushed the PhilCoggins:remove_item_subtype_req_for_limit branch from d79d6d6 to bca9aa5

Rebasing on master as development dependencies have been updated. Also, I've added a minimum test coverage requirement.

@PhilCoggins
Copy link
Contributor Author

@jaredbeck I'll get this updated early this week, today or tomorrow evening.

@PhilCoggins
Removes item_subtype requirement for model-specific limits.
12c0525 
* When enforcing limits for a model, it was previously required to
  have the item_subtype column on the versions table to obtain the
  configured limit for a given model type, especially for models
  subclassed via STI.

* This change instead references the item on the version record,
  which is already available via the association cache, to determine
  the properly configured limit. This change drops the requirement of
  having item_subtype to configure model-specific limits.
@PhilCoggins PhilCoggins force-pushed the remove_item_subtype_req_for_limit branch from bca9aa5 to 12c0525 Compare July 26, 2021 15:05
@PhilCoggins
Copy link
Contributor Author

@jaredbeck Updated

@jaredbeck jaredbeck self-requested a review July 26, 2021 15:27
@jaredbeck jaredbeck merged commit 7bbf69a into paper-trail-gem:master Jul 26, 2021
@jaredbeck
Copy link
Member

Thanks Phil, nice work

@PhilCoggins PhilCoggins deleted the remove_item_subtype_req_for_limit branch July 26, 2021 15:38
@jaredbeck
Copy link
Member

Released in 12.1.0

@Niordsid Niordsid mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredbeck jaredbeck jaredbeck approved these changes

@aried3r aried3r Awaiting requested review from aried3r

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to load schema with limit option
3 participants
@PhilCoggins @jaredbeck @aried3r